cubeops: propagate database errors from the settings and user getters - #1382
cubeops: propagate database errors from the settings and user getters#1382dwin-gharibi wants to merge 9 commits into
Conversation
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
| if errors.Is(err, sql.ErrNoRows) || val == "" { | ||
| return "", nil | ||
| if err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { |
There was a problem hiding this comment.
This sql.ErrNoRows branch can't fire with the current GORM setup, so the "absent" case is actually detected by the fall-through below.
db.Raw(...).Scan(&val) goes through GORM's Scan (not *sql.Row), and GORM returns a nil error when the result set is empty — with the default &gorm.Config{} (no TranslateError) used in CubeDB/dao/driver/mysql/mysql.go:61, it never surfaces sql.ErrNoRows. That's why the codebase's other getters that genuinely need to detect a missing row use .Row().Scan(...) instead (refresh_token.go:33-35, agenthub.go:229-231). The old code's val == "" clause was the effective check, and here the err == nil path returning the zero value (return val, nil) is what keeps "absent → ("", nil)" working.
This is harmless (behavior is correct) and even a forward-compatible safety net if GORM ever changes, so no change is strictly required — but the PR description presents this branch as the mechanism that separates "absent" from "error", which isn't what happens today. A one-line comment noting the empty-value fall-through is the real path would prevent a future reader from "fixing" it. Same applies to the identical branches in GetSetting and GetUserPassword.
| } | ||
|
|
||
| func TestChangePasswordSurfacesInfrastructureError(t *testing.T) { | ||
| dbDown := errors.New("context canceled") |
There was a problem hiding this comment.
This test is weaker than its Login sibling in two ways:
-
It never asserts the underlying error is preserved.
TestLoginSurfacesInfrastructureErrorcheckserrors.Is(err, dbDown), but here onlyerr != nil && !errors.Is(err, ErrInvalidOldPassword)is asserted. A regression to a different non-sentinel error that discards the cause (e.g.errors.New("read failed")) would still pass. Consider addingif !errors.Is(err, dbDown) { t.Fatalf(...) }for parity. -
errors.New("context canceled")is a misleading stand-in for the "database unreachable" case — a context cancellation is a different failure mode and could mask a wrong reason. The Login test's"dial tcp ...: connection refused"is a better fit.
Review: PR #1382 — cubeops: propagate database errors from the settings and user gettersAI-generated review; not a human approval. Overall assessmentThis is a well-scoped and correct fix. The root cause is accurately diagnosed: The handler-layer change (returning a generic No blocking correctness issues found. The points below are minor. Findings
Non-blocking notes
TestingThe added tests are appropriate and meaningful: store-contract tests exercise the absent-row / driver-error / empty-value tri-state against a real MySQL container; the service tests pin the enumeration-safety and error-propagation behavior; the handler tests verify no driver detail leaks on the three 500 paths; and the e2e outage test validates the wire-level behavior with the database killed mid-run. The |
|
Thanks for this PR.
|
… the settings and user getters Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
…proper tests for auth db errors testing Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
…ws really signals an absent row Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
…-vs-error getter contract Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
…n 500 responses Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
…dies leak no database detail Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
…500 paths leaking error detail Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
…fresh 500 body leaks nothing Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
9b3d06a to
27b92a9
Compare
|
Everything is fine now @liciazhu. |
|
LGTM. |
…ing server and check the 500 bodies Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
| // Validation errors and DB errors share the 500 path here; finer | ||
| // mapping can be added by wrapping with sentinel errors if needed. | ||
| httputil.WriteError(c, http.StatusInternalServerError, err.Error()) | ||
| httputil.WriteError(c, http.StatusInternalServerError, "internal server error") |
There was a problem hiding this comment.
The switch to a generic body here also masks client-side validation errors, not just DB errors. service.ChangePassword returns plain errors for "oldPassword and newPassword are required" and "new password must be at least 4 characters", which land in this default branch — so a client sending a too-short new password now gets an indistinguishable 500 {"error":"internal server error"} instead of the previous message. The 500 status for these was already pre-existing, but the message at least told the client what to fix. Consider mapping the validation errors to 400 (or wrapping them in a sentinel) so they don't share the DB-failure body — the comment above already notes this conflation.
| }) | ||
|
|
||
| t.Run("refresh does not leak driver detail", func(t *testing.T) { | ||
| code, body := do(t, http.MethodPost, inst.baseURL+"/api/v1/auth/refresh", "", |
There was a problem hiding this comment.
These two subtests only t.Logf when the status isn't 500, so they pass even if change-password/refresh return 401 or 200 during the outage (as long as the body carries no driver detail). Both paths deterministically hit the DB while the server is up (GetUserPassword for change-password, IsRefreshTokenRevoked for refresh) and the auth middleware is signature-only, so both should reliably return 500. For consistency with the login subtest above, asserting code == http.StatusInternalServerError here would make the guarantee uniform.
| return inst | ||
| } | ||
| } | ||
| if cmd.ProcessState != nil && cmd.ProcessState.Exited() { |
There was a problem hiding this comment.
cmd.ProcessState is only populated by Wait(), which is never called in this readiness loop, so cmd.ProcessState != nil is always false here — this crash-detection branch never fires. If cubeops exits early, the harness keeps polling /health until the 4-minute deadline and only then reports the failure via the log tail, instead of failing fast. Also, do() uses http.DefaultClient with no timeout, so if the server hangs during an outage the test stalls indefinitely. A short HTTP timeout (and waiting on the process in a goroutine, or polling cmd.Process liveness) would make the harness fail faster and more reliably.
Closes #1381.
Motivation
GetSystemSetting,GetSettingandGetUserPasswordcollapsed three different outcomes — rowmissing, value empty, and any database error — into
("", nil). Because the scanned value is thezero value whenever the query fails,
val == ""was true for a real error too, so the error wasdiscarded.
The visible symptom is that a database outage is reported as
401 invalid credentialsrather than a500, which is the opposite of whatAuthService.Login's own comment promises. It also made bothbranches of
if err != nilinLoginunreachable dead code.What this changes
CubeOps/internal/store/setting.go— all three getters now separate the two cases:The resulting contract is:
("", nil)means absent; a non-nil error means the read failed. Anempty stored value is still reported as absent, which is what every caller already wanted.
CubeOps/internal/service/auth.go—LoginandChangePasswordupdated for that contract:Loginreturns500for a read failure andErrInvalidCredentialswhen the user is absent(
stored == "") or the password does not match. User enumeration is still not possible: absentand wrong-password are indistinguishable to the caller.
ChangePasswordlikewise returns the wrapped error for a read failure andErrInvalidOldPasswordwhen the user is absent or the old password is wrong.CubeOps/internal/service/auth_test.go— thefakeUserStorereturned("", errors.New("user not found"))for an unknown user, modelling not-found as an error, whilethe real store returns
("", nil). That mismatch is why the tests passed against a contractproduction never implemented. The fake now returns
("", nil); its assertion (unknown user →ErrInvalidCredentials) is unchanged and still passes.Callers that already discard the error (
internal/service/openclaw.go:685-701,internal/service/agenthub.go:457) are untouched — they still get""and still ignore the error, sotheir behaviour is unchanged.
bootstrapMasterKey(internal/store/db.go:83,93) andBootstrapJWTSecret(:139) already hadif err != nilguards that were previously dead; they now actually fire, which is the intendedfail-closed behaviour.
Testing
New:
CubeOps/internal/service/auth_db_error_test.goTestLoginSurfacesInfrastructureError— a DB error is wrapped and returned, and is notErrInvalidCredentials.TestLoginUnknownUserStillReportsInvalidCredentials— no enumeration regression.TestChangePasswordSurfacesInfrastructureError— same for the password-change path.TestChangePasswordUnknownUserReportsBadOldPassword— absent user still reports a bad oldpassword.
CI gates checked locally:
gofmt -l ./internal ./cmd— clean (fmt-check).go build ./...— clean.go test ./...— 0 failures (unit-test-check→make cubeops-test).Risk / rollout
This is a behaviour change on the login path and is the reason it is split out from #1: during a
database outage,
/api/v1/auth/loginnow returns500instead of401. Any monitoring or clientretry logic keyed on
401for that case will see500instead — which is the correct signal, butworth calling out.
The
UserStorecontract change is internal to CubeOps (internal/), so there is no external APIimpact.